Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validator 'declared_only' to reject undeclared params #1606

Closed
wants to merge 2 commits into from

Conversation

jlfaber
Copy link
Contributor

@jlfaber jlfaber commented Apr 4, 2017

See issues #810, #1603, and previous PR #1158.
This implementation only checks the current scope, so nested scopes will need to repeat the method. This was both easier to implement and more flexible since it allows for some params to be locked down and others to be flexible.

@dblock
Copy link
Member

dblock commented Apr 4, 2017

I really like where this is going, in fact I love it, but before we merge this maybe we can do even better?

Lets say I want to enable for an entire API. How would I do that?

I feel like this is a DSL opportunity. For example declared params do ... or required params do or only params do or even strong params do? What do you think?

@jlfaber
Copy link
Contributor Author

jlfaber commented Apr 4, 2017

Doing this at a top level for an entire API (across multiple endpoints) is an interesting idea for those who really want to lock things down. But I think you meant applying it at the level of a single param block and all subordinate blocks. If we were going to do that, I'd probably opt for something like params allow_undeclared: false do. (Similar to allow_blank for individual params.)

That said, I've just realized that there's a bug with route_param that I need to fix -- so don't merge this yet!

@dblock
Copy link
Member

dblock commented Apr 7, 2017

I like allow_undeclared: true/false a lot more, thanks @jlfaber. Don't love the name but don't feel strongly about it. Maybe allow_any? Again no strong feelings.

@dblock
Copy link
Member

dblock commented Apr 7, 2017

I did mean for the entire API. If I were building a brand new service I'd want this to be the default behavior of all my params. I would do this through an extension:

class MyAPI < Grape::API
   include Grape::Params::AllowAny::False

end

@dblock
Copy link
Member

dblock commented Jun 14, 2017

@jlfaber Can I help with this? Lets finish it for 1.0?

@seanhandley
Copy link

This is awesome, and would help our QA team a lot with mistakes in cURL payloads when testing endpoints.

Is there anything I can do to help move this towards completion?

@jlfaber
Copy link
Contributor Author

jlfaber commented Jan 17, 2018

If anyone would like to pick this up, I'm absolutely good with it. I put the brakes on earlier because (IIRC) there were still issues dealing with inherited params (route params and explicit ones), especially when using mounted APIs. It's a bit fuzzy.

We ended up going in a different direction (JSON Schema) to accomplish what we needed so I can't prioritize working this issue right now. So if someone would like to pick up the torch, you absolutely have my blessing! Sorry to have left this hanging so long!

@seanhandley
Copy link

No worries @jlfaber - thanks for the info!

I'll see about carrying on the work you started.

@seanhandley
Copy link

I've rebased your branch with master and raised a new PR at #1729 - this PR can be closed.

@jlfaber
Copy link
Contributor Author

jlfaber commented Jan 17, 2018

Awesome! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants